Add Thomas Algorithm in Rust #693
Conversation
This one is implemented without any external crates, so you can compile it just with `rustc`.
| @@ -0,0 +1,38 @@ | |||
| // This implementation is based on the C++ implementation. | |||
There was a problem hiding this comment.
With your and @leios's blessing, I would probably go on a crusade to remove all such comments (e.g. "This implementation was written by @strega-nil") throughout the codebase.
There was a problem hiding this comment.
Honestly, I think that's a fine idea.
They were originally implemented as a way of letting readers know that this code could have been written by someone who did not write previous implementations in the same language.
Now that the review process is more rigorous, it doesn't have as much meaning.
| @@ -0,0 +1,38 @@ | |||
| // This implementation is based on the C++ implementation. | |||
|
|
|||
| fn thomas(a: &[f64], b: &[f64], c: &[f64], x: &mut Vec<f64>) { | |||
There was a problem hiding this comment.
If you change the &[f64] to a Vec<f64> you can get rid of the references in the main which would be a bit cleaner.
| @@ -0,0 +1,38 @@ | |||
| // This implementation is based on the C++ implementation. | |||
|
|
|||
| fn thomas(a: &[f64], b: &[f64], c: &[f64], x: &mut Vec<f64>) { | |||
There was a problem hiding this comment.
I would also probably return a Vec<64> instead of mutating the Vec. Makes it a bit more cleaner to read imho.
so in the end i think the best signature would be fn thomas(a: Vec<f64>, b: Vec<f64>, c: Vec<f64>, mut x: Vec<f64>) -> Vec<f64>
There was a problem hiding this comment.
I think we agreed in #510 that we want functions to be pure, even though at least the Thomas algorithm functions are all over the place in that regard. With that in mind, I think the best signature is
fn thomas(a: &[f64], b: &[f64], c: &[f64], d: &[f64]) -> Vec<f64> {so that you have the efficiency of the function not taking ownership of its arguments, but also being pure.
berquist
added
the
Implementation
I am a fan of pure functions, so I redid the function so that it returns a Vec<f64> rather than mutating a slice.
| let y = thomas(&a, &b, &c, &x); | ||
|
|
||
| y.iter() | ||
| .for_each(|i| println!("[{:>19}]", format!("{:18}", format!("{:?}", i)))); |
There was a problem hiding this comment.
One question before I override @Liikt and approve. Why is this nested and not just a single println!?
There was a problem hiding this comment.
I essentially wanted to pretty-print the result in a certain way. (I want positive values to lead with a space.) If you want, I can get rid of the pretty-printing.
There was a problem hiding this comment.
Isn't this the same?
y.iter().for_each(|i| println!("[{:>19}]", i));There was a problem hiding this comment.
Not quite. The result I was looking for satisfies these guidelines:
- The sign should either be a negative or a space.
- The decimal point should (ideally) be aligned.
I guess we don't have to pretty-print the result if we don't have to.
There was a problem hiding this comment.
I understand better now. As long as the explanation is in the PR thread like this, it's ok.
berquist
dismissed
Liikt’s stale review
Going with the implementation style given in #510
For leios: This one is implemented without any external crates, so you can compile it just with
rustc. (Let me know if what I did for pretty-printing on line 37 of the code was too much.)